Expand service config support #1165

lyuxuan · 2017-04-03T22:16:26Z

Add client and server APIs to limit send/recv message size.
Add logic to handle scenarios where both API and service config config the same field.
Update MethodConfig Struct.

… Update MethodConfig struct

menghanl · 2017-04-04T22:21:27Z

call.go

+const defaultClientMaxReceiveMessageSize = 1024 * 1024 * 4
+const defaultClientMaxSendMessageSize = 1024 * 1024 * 4
+
+func min(a, b int) int {


move to rpc_util.go

menghanl · 2017-04-04T22:25:49Z

clientconn.go

 }

 const defaultClientMaxMsgSize = math.MaxInt32

 // DialOption configures how we set up the connection.
 type DialOption func(*dialOptions)

-// WithMaxMsgSize returns a DialOption which sets the maximum message size the client can receive.
+// WithMaxMsgSize returns a DialOption which sets the maximum message size the client can receive. This function is for backward API compatibility. It has essentially the same functionality as WithMaxReceiveMessageSize.


Deprecated: use WithMaxReceiveMessageSize instead.
And return WithMaxReceiveMessageSize(s).

menghanl · 2017-04-06T00:05:37Z

call.go

@@ -122,6 +122,9 @@ func sendRequest(ctx context.Context, dopts dialOptions, compressor Compressor,
 	if err != nil {
 		return nil, Errorf(codes.Internal, "grpc: %v", err)
 	}
+	if len(outBuf) > msgSizeLimit {
+		return nil, Errorf(codes.InvalidArgument, "Sent message larger than max (%d vs. %d)", len(outBuf), msgSizeLimit)


gRPC itself should not return code InvaludArgument.
(If this is consistent with other languages, probably we should comment that).

menghanl · 2017-04-06T00:12:06Z

call.go

@@ -147,15 +150,49 @@ func Invoke(ctx context.Context, method string, args, reply interface{}, cc *Cli
 	return invoke(ctx, method, args, reply, cc, opts...)
 }

+const defaultClientMaxReceiveMessageSize = 1024 * 1024 * 4
+const defaultClientMaxSendMessageSize = 1024 * 1024 * 4


Move those to clientconn.go, around line 106? Since they are shared by Unary and Streaming.
Remove "client" from the variable name.
And do

const ( defaultMaxReceiveMessageSize = 1024 * 1024 * 4 defaultMaxSendMessageSize = 1024 * 1024 * 4 )

menghanl · 2017-04-06T00:12:56Z

clientconn.go

+	scChan                <-chan ServiceConfig
+	copts                 transport.ConnectOptions
+	maxReceiveMessageSize int
+	maxSendMessageSize    int
 }

 const defaultClientMaxMsgSize = math.MaxInt32


Remove this const if it's not used.

menghanl · 2017-04-06T00:42:19Z

server.go

@@ -164,10 +167,26 @@ func RPCDecompressor(dc Decompressor) ServerOption {
 }

 // MaxMsgSize returns a ServerOption to set the max message size in bytes for inbound mesages.
-// If this is not set, gRPC uses the default 4MB.
+// If this is not set, gRPC uses the default 4MB. This function is for backward compatability. It has essentially the same functionality as MaxReceiveMessageSize.


Deprecated: ...
And return MaxReceiveMessageSize(m)

menghanl · 2017-04-06T00:42:58Z

clientconn.go

+	}
+}
+
+// WithMaxReceiveMessageSize returns a DialOption which sets the maximum message size the client can receive. Negative input is invalid and has the same effect as not setting the field.


Mention the default value as the server side?

Done. server defaults has been moved from server.go to clientconn.go:109

menghanl · 2017-04-06T00:44:43Z

server.go

@@ -629,6 +649,9 @@ func (s *Server) sendResponse(t transport.ServerTransport, stream *transport.Str
 		// the optimal option.
 		grpclog.Fatalf("grpc: Server failed to encode response %v", err)
 	}
+	if len(p) > s.opts.maxSendMessageSize {
+		return Errorf(codes.InvalidArgument, "Sent message larger than max (%d vs. %d)", len(p), s.opts.maxSendMessageSize)


InvalidArgument is also weird here. This is to send response...

menghanl · 2017-04-06T00:45:25Z

server.go

 				// TODO: Revisit the error code. Currently keep it consistent with
 				// java implementation.
-				return status.Errorf(codes.Internal, "grpc: server received a message of %d bytes exceeding %d limit", len(req), s.opts.maxMsgSize)
+				return status.Errorf(codes.InvalidArgument, "grpc: server received a message of %d bytes exceeding %d limit", len(req), s.opts.maxReceiveMessageSize)


The error message is not updated for this.

menghanl · 2017-04-06T00:47:11Z

stream.go

@@ -576,6 +610,9 @@ func (ss *serverStream) SendMsg(m interface{}) (err error) {
 		err = Errorf(codes.Internal, "grpc: %v", err)
 		return err
 	}
+	if len(out) > ss.maxSendMessageSize {
+		return Errorf(codes.InvalidArgument, "Sent message larger than max (%d vs. %d)", len(out), ss.maxSendMessageSize)


And the error code InvalidArgument here, too.
This is to send response...

dfawley · 2017-04-21T16:45:46Z

call.go

@@ -52,7 +52,7 @@ import (
 //
 // TODO(zhaoq): Check whether the received message sequence is valid.
 // TODO ctx is used for stats collection and processing. It is the context passed from the application.
-func recvResponse(ctx context.Context, dopts dialOptions, t transport.ClientTransport, c *callInfo, stream *transport.Stream, reply interface{}) (err error) {
+func recvResponse(ctx context.Context, dopts dialOptions, msgSizeLimit int, t transport.ClientTransport, c *callInfo, stream *transport.Stream, reply interface{}) (err error) {


Consider passing the method config instead so that we don't need to continue extending the parameter list here.

dfawley · 2017-04-21T18:20:03Z

call.go

 			defer cancel()
 		}
+
+		if mc.MaxReqSize != nil && cc.dopts.maxSendMessageSize >= 0 {


How about something like this:

func getMaxSize(mcMax *int, doptMax, default int) int { if mcMax == nil && doptMax < 0 { return default } if mcMax != nil && doptMax >= 0 { return min(*mcMax, doptMax) } if mcMax != nil { return *mcMax } return doptMax }

then call that to compute both send and receive limits?

dfawley · 2017-04-21T18:29:49Z

clientconn.go

+}
+
+// WithMaxReceiveMessageSize returns a DialOption which sets the maximum message size the client can receive. Negative input is invalid and has the same effect as not setting the field.
+func WithMaxReceiveMessageSize(s int) DialOption {


Should these be call options instead?

I think the abstraction of restricting configuration that applies to calls to CallOptions and then providing a "default call option" as a dial option is nice. I.e.

WithDefaultCallOptions(cos ...CallOption) DialOption { return func (o *dialOptions) { o.callOptions = append(o.callOptions, cos...) } }

It makes it slightly more cumbersome to set it when dialing, but it also provides more flexibility.

My recommendation is to add the above and then make WithMax{Send,Receive}MessageSize return CallOption instead of DialOption.

The legacy WithMaxMsgSize could still be provided to avoid an API breaking change, but it could be implemented by o.callOptions = append(o.callOptions, WithMaxReceiveMessageSize(s)).

dfawley · 2017-04-21T18:31:19Z

call.go

+			maxReceiveMessageSize = cc.dopts.maxReceiveMessageSize
+		}
+	} else {
+		if cc.dopts.maxSendMessageSize >= 0 {


If cc.GetMethodConfig returned an empty MethodConfig (instead of ok=false), then this wouldn't need to be special-cased. There would be nils for these fields and the logic above would apply. Is that feasible?

dfawley · 2017-04-21T20:28:28Z

clientconn.go

-	cc.dopts.maxMsgSize = defaultClientMaxMsgSize
+
+	// initialize maxReceiveMessageSize and maxSendMessageSize to -1 before applying DialOption functions to distinguish whether the user set the message limit or not.
+	cc.dopts.maxReceiveMessageSize = -1


I agree; nil pointers are a better signal of not-set than magic values.

menghanl · 2017-04-26T23:39:19Z

clientconn.go

+const (
+	defaultClientMaxReceiveMessageSize = 1024 * 1024 * 4
+	defaultClientMaxSendMessageSize    = 1024 * 1024 * 4
+	defaultServerMaxReceiveMessageSize = 1024 * 1024 * 4


Move the two variables for server to server.go?

menghanl · 2017-04-26T23:46:29Z

rpc_util.go

@@ -475,4 +494,24 @@ const SupportPackageIsVersion4 = true
 // Version is the current grpc version.
 const Version = "1.3.0-dev"

+func min(a, b *int) *int {


Nit: move these two new functions before the const SupportPackageIsVersion4.

menghanl · 2017-04-26T23:50:50Z

stream.go

+	}
+
+	if mc.Timeout != nil && *mc.Timeout >= 0 {
+		var cancel context.CancelFunc


cancel was defined at line 113.
And this cancel shouldn't be called when newClientStream returns.
It will be stored in the stream at line 228, and will be called when the stream finishes.

dfawley · 2017-05-05T20:42:07Z

Please resolve the conflicts and we will make sure to prioritize the re-review of this.

dfawley · 2017-05-09T21:49:57Z

I notice that this code is still using a Go struct instead of JSON for the service config. This means that it's not easily extensible -- i.e., it doesn't provide a way for third parties to add new fields to be used in their own interceptors. I assume that we're going to fix that in a separate PR?

@markdroth - We also aren't passing the service config to the interceptors. Are these requirements?

markdroth · 2017-05-09T21:51:21Z

@dfawley Ultimately, yes, those are requirements. The design was intended to be extensible by third parties.

… issues

menghanl

Mostly about naming and comment.

menghanl · 2017-05-17T22:34:23Z

call.go

+		return Errorf(codes.Internal, "callInfo maxSendMessageSize field uninitialized(nil)")
+	}
+	if len(outBuf) > *c.maxSendMessageSize {
+		return Errorf(codes.ResourceExhausted, "Sent message larger than max (%d vs. %d)", len(outBuf), *c.maxSendMessageSize)


Nit: this is not sent message.
How about trying to send message larger than ...?

C implementation use this sentence. Do we want to be consistent with them or not?
https://github.com/grpc/grpc/blob/master/src/core/ext/filters/message_size/message_size_filter.c#L141

menghanl · 2017-05-17T22:41:23Z

clientconn.go

@@ -123,10 +126,15 @@ func WithInitialConnWindowSize(s int32) DialOption {
 	}
 }

-// WithMaxMsgSize returns a DialOption which sets the maximum message size the client can receive.
+// WithMaxMsgSize returns a DialOption which sets the maximum message size the client can receive. Deprecated: use WithMaxReceiveMessageSize instead.


WithMaxReceiveMessageSize is not a DialOption anymore.
We could say, "use WithDefaultCallOptions(WithMaxReceiveMessageSize(s)) instead",
or "use CallOption WithDefaultCallOptions instead".

Also, see my following comment, WithDefaultCallOptions should be renamed.

menghanl · 2017-05-17T22:42:42Z

rpc_util.go

@@ -209,6 +211,22 @@ func FailFast(failFast bool) CallOption {
 	})
 }

+// WithMaxReceiveMessageSize returns a CallOption which sets the maximum message size the client can receive.
+func WithMaxReceiveMessageSize(s int) CallOption {


Rename this to MaxReceiveMessageSize, by convention we don't name CallOption WithSomething.

menghanl · 2017-05-17T22:43:18Z

rpc_util.go

+}
+
+// WithMaxSendMessageSize returns a CallOption which sets the maximum message size the client can send.
+func WithMaxSendMessageSize(s int) CallOption {


Rename to MaxSendMessageSize.

menghanl · 2017-05-17T22:46:12Z

server.go

@@ -125,7 +131,7 @@ type options struct {
 	initialConnWindowSize int32
 }

-var defaultMaxMsgSize = 1024 * 1024 * 4 // use 4MB as the default message size limit
+var defaultServerOptions = options{maxReceiveMessageSize: defaultServerMaxReceiveMessageSize, maxSendMessageSize: defaultServerMaxSendMessageSize}


Format

var defaultServerOptions = options{ maxReceiveMessageSize: defaultServerMaxReceiveMessageSize, maxSendMessageSize: defaultServerMaxSendMessageSize, }

menghanl · 2017-05-17T22:48:33Z

server.go

+	return MaxReceiveMessageSize(m)
+}
+
+// MaxReceiveMessageSize returns a ServerOption to set the max message size in bytes for inbound mesages.


The comment,
... max message size in bytes the server can receive., since we name the option ...Receive....

menghanl · 2017-05-17T22:48:51Z

server.go

+	}
+}
+
+// MaxSendMessageSize returns a ServerOption to set the max message size in bytes for outbound mesages.


Similar to the above comment
... the server can send.

menghanl · 2017-05-17T22:50:22Z

stream.go

+		return Errorf(codes.Internal, "callInfo maxSendMessageSize field uninitialized(nil)")
+	}
+	if len(out) > *cs.c.maxSendMessageSize {
+		return Errorf(codes.ResourceExhausted, "Sent message larger than max (%d vs. %d)", len(out), *cs.c.maxSendMessageSize)


trying to send...

menghanl · 2017-05-17T22:50:57Z

stream.go

@@ -591,6 +612,9 @@ func (ss *serverStream) SendMsg(m interface{}) (err error) {
 		err = Errorf(codes.Internal, "grpc: %v", err)
 		return err
 	}
+	if len(out) > ss.maxSendMessageSize {
+		return Errorf(codes.ResourceExhausted, "Sent message larger than max (%d vs. %d)", len(out), ss.maxSendMessageSize)


trying to send...

dfawley · 2017-05-16T19:45:25Z

call.go

@@ -73,7 +73,11 @@ func recvResponse(ctx context.Context, dopts dialOptions, t transport.ClientTran
 		}
 	}
 	for {
-		if err = recv(p, dopts.codec, stream, dopts.dc, reply, dopts.maxMsgSize, inPayload); err != nil {
+		if c.maxReceiveMessageSize == nil {
+			// TODO(lyuxuan): codes.Internal the right error to return here?


SGTM. A broken invariant is an internal problem. (Please remove comment.)

dfawley · 2017-05-16T19:47:18Z

call.go

+			// TODO(lyuxuan): codes.Internal the right error to return here?
+			return Errorf(codes.Internal, "callInfo maxReceiveMessageSize field uninitialized(nil)")
+		}
+		if err = recv(p, dopts.codec, stream, dopts.dc, reply, *c.maxReceiveMessageSize, inPayload); err != nil {


Pass "c" instead?

recv() is also used by serverStream

dfawley · 2017-05-16T19:48:09Z

call.go

@@ -118,6 +122,13 @@ func sendRequest(ctx context.Context, dopts dialOptions, compressor Compressor,
 	if err != nil {
 		return Errorf(codes.Internal, "grpc: %v", err)
 	}
+	if c.maxSendMessageSize == nil {
+		// TODO(lyuxuan): codes.Internal the right error to return here?


Remove TODO; this is fine.

dfawley · 2017-05-19T22:59:57Z

call.go

+	if c.maxSendMessageSize == nil {
+		return Errorf(codes.Internal, "callInfo maxSendMessageSize field uninitialized(nil)")
+	}
+	if len(outBuf) > *c.maxSendMessageSize {


This ideally should be pushed into encode() instead. Otherwise, we will still be allocating more memory than desired. encode() gets the size before it allocates the buffer, so we can safe the work of encoding the object if we check it there after we get the size.

dfawley · 2017-05-19T23:05:51Z

stream.go

+		c.failFast = !*mc.WaitForReady
+	}
+
+	if mc.Timeout != nil && *mc.Timeout >= 0 {


> 0? A timeout of zero seems like a problem.

menghanl

LGTM.

dfawley

LGTM

Add client and service side apis for limiting the send/recv msg size.…

50d4175

… Update MethodConfig struct

lyuxuan requested review from menghanl, MakMukhi and dfawley April 3, 2017 22:16

lyuxuan added 6 commits April 3, 2017 15:20

merge master

13b5f12

fix missing import

a0b902a

fix gofmt goimports go tool vet errors

ad16b94

Make initial service config non-blocking in Dial()

a66f923

Merge branch 'master' into service_config_pr

f02290b

gofmt

f1bb70f

menghanl reviewed Apr 6, 2017

View reviewed changes

lyuxuan added 5 commits April 6, 2017 14:08

fix the testMaxMsgSizeServerAPI failure

6f8b553

Merge branch 'master' into service_config_pr

fa29686

fix send response error case

c6a3937

merge master resolve conflicts

8788b75

change error message from InvalidArgument to ResourceExhausted

cb02ab4

dfawley reviewed Apr 21, 2017

View reviewed changes

lyuxuan added 4 commits April 21, 2017 16:07

merge master

bab6b61

update the merge of client api and sc

983d837

make max size a pointer type and initialize function a CallOption

9c5f260

minor comment change

eaa9ccb

menghanl reviewed Apr 26, 2017

View reviewed changes

lyuxuan added 2 commits April 26, 2017 17:39

move server defaults, delete defer cancel() in stream.go

ecbc34a

update

ea230c7

dfawley assigned lyuxuan May 5, 2017

lyuxuan added 3 commits May 7, 2017 16:49

Merge branch 'master' into service_config_pr

3ea2870

remove unessary nil in return statement

d926544

gofmt

59426b3

lyuxuan removed their assignment May 8, 2017

dfawley assigned lyuxuan and unassigned menghanl and dfawley May 11, 2017

dfawley mentioned this pull request May 11, 2017

Support multiple encodings #1203

Closed

lyuxuan added 4 commits May 15, 2017 13:51

add timeout test, add check or pointer filed in callOption, fix minor…

bdf9a64

… issues

merge master, resolve conflicts

35d77ea

fix minor typo

4d2b4b5

comments added

7505481

lyuxuan force-pushed the service_config_pr branch from d8f6569 to 7505481 Compare May 15, 2017 22:14

lyuxuan assigned menghanl and dfawley and unassigned lyuxuan May 15, 2017

menghanl reviewed May 17, 2017

View reviewed changes

lyuxuan added 3 commits May 18, 2017 14:52

merge master

504db8e

change max message size functions name

d19bbe8

remove unnecessary ok

ed64d51

dfawley reviewed May 19, 2017

View reviewed changes

lyuxuan added 2 commits May 19, 2017 14:55

remove some todo comments

27ae147

fix minor issues

cb64938

dfawley reviewed May 19, 2017

View reviewed changes

minor fix

4a7b4d0

menghanl approved these changes May 20, 2017

View reviewed changes

dfawley approved these changes May 22, 2017

View reviewed changes

lyuxuan merged commit 9f919f7 into grpc:master May 22, 2017

menghanl added 1.4 Type: Feature New features or improvements in behavior labels Jun 7, 2017

thaJeztah mentioned this pull request Oct 30, 2018

Update containerd to 1.1 moby/moby#36895

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Expand service config support #1165

Expand service config support #1165

Conversation

lyuxuan commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented May 5, 2017

dfawley commented May 9, 2017

markdroth commented May 9, 2017

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment